Repository navigation
Conversation
) A mode solve asked for many modes around a lossy conductor also returns modes confined to the metal, with |n_eff| near |sqrt(eps_metal)|. Some lie inside the loss bound, and the slowest-mode rule of select_line_mode picked one, reading R' about 1e9 off. Add max_index to propagating_modes and select_line_mode (default None, selection unchanged) and gsim.femwell.conductor_index_bound, which derives a bound from the permittivity given to solve_modes.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f6f30c33e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| conductors = values[np.abs(values.imag) > np.abs(values.real)] | ||
| if conductors.size == 0: | ||
| return None | ||
| return float(fraction * np.sqrt(np.abs(conductors)).min()) |
There was a problem hiding this comment.
For a conduction-dominated but only moderately conductive region—for example, a doped-silicon element with epsilon=11.7-12j—this predicate includes it and the default calculation returns about 0.41. Passing that documented result to select_line_mode(..., max_index=...) combines it with the default min_index=1, so every possible default candidate is removed; in a metal-plus-graded-semiconductor map, the min() similarly lets one near-threshold element collapse an otherwise useful metal cutoff. Avoid returning a cutoff below the candidate floor (or require enough separation/an expected line index) rather than treating every |Im(epsilon)| > |Re(epsilon)| value as a usable bound.
AGENTS.md reference: AGENTS.md:L51-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 790c2da. A conduction-dominated region now sets the bound only when fraction * |sqrt(eps)| is at least margin (default 10) times the largest index of the non-conducting regions, and never below the light line. Regions that fall short are skipped with a UserWarning, and the function returns None when none qualifies. 11.7-12j alone now gives None with a warning instead of 0.41, and next to aluminium it no longer lowers the aluminium bound (tests in TestConductorIndexBound).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
==========================================
+ Coverage 91.61% 91.63% +0.02%
==========================================
Files 204 204
Lines 28179 28283 +104
Branches 5324 5350 +26
==========================================
+ Hits 25816 25917 +101
Misses 1354 1354
- Partials 1009 1012 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Alisama20
left a comment
Alisama20
left a comment
There was a problem hiding this comment.
tests/common and tests/femwell pass on 5f6f30c. TestManyModes bites: with 12 modes and no bound the pick is 27921-1548j and R' is 3.8e7 times too high. With the bound it is within 1e-4 of 1/(sigma W t).
The Codex P2 holds. The min over every region with |Im eps| > |Re eps| lets a weak conductor set the bound. 11.7-12j gives 0.41 and select_line_mode raises NoLineModeError. 11.7-1000j gives 3.16, and with modes [5.13-4.68j, 2.5-0.01j] the 2.5 mode is picked without a warning.
|Im eps| > |Re eps| says conduction dominates, not how far |sqrt(eps)| sits above the line index, which is what the bound needs. I would let a region set the bound only when 0.1*|sqrt(eps)| clears the dielectric index by a wide margin, skip and warn otherwise, and never return a bound below min_index. A test with 11.7-12j should go with it.
For Al, Cu and W at 1 to 100 GHz the margin is 100x or more. For a 5e5 S/m film like TiN it is about 5x.
Without max_index the results match main on 20000 random mode sets.
A conduction-dominated region with |sqrt(eps)| near the line index, such as doped silicon, collapsed the bound: 11.7-12j gave 0.41, below the light line, and 11.7-1000j gave 3.16, dropping a 5.13-4.68j line mode. A conductor now sets the bound only when fraction * |sqrt(eps)| reaches margin (default 10) times the largest non-conductor index; others are skipped with a warning.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 790c2da34e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) | ||
| if not usable.any(): | ||
| return None | ||
| return float(bounds[usable].min()) |
There was a problem hiding this comment.
When a many-mode solve contains both Al and the skipped doped-Si value from the new test (epsilon={"al": 1-6.8e8j, "si": 11.7-1000j, "oxide": 3.9}), this return uses only Al and yields max_index ≈ 2608. A Si-confined mode near sqrt(eps_si) = 22.49-22.23j passes both the default loss bound (0.988 < 1) and this index bound, and its real index exceeds that of the 5.13-4.68j line mode, so the documented select_line_mode(..., max_index=...) flow can still select the material mode. The warning does not make the returned partial cutoff safe; either account for the skipped conductor in the cutoff or avoid returning one when such a mode can survive.
AGENTS.md reference: AGENTS.md:L51-L53
Useful? React with 👍 / 👎.
|
Thanks @Alisama20, fixed in 790c2da along the lines you suggested.
For a 5e5 S/m film like TiN, the bound depends on the surroundings: at 100 GHz it is 30, which passes next to oxide alone (needs 19.7) and is skipped with a warning next to silicon (needs 34). |
|
I re-ran tests/common and tests/femwell on 790c2da and they pass (1102 passed, 2 skipped). |
Fixes #498.
Problem
select_line_modepicks the propagating mode with the highestRe(n_eff). When a femwell solve is asked for more than a few modes around a lossy conductor, it also returns modes confined to the metal, with|n_eff|close to|sqrt(eps_metal)|(about 2.6e4 for aluminium at 1 GHz). Some of them lose less per unit length than they advance, so the loss bound keeps them, and one is selected over the line mode. On the coupled-strip geometry from #483 this gave R′ about 1e9 off with no warning. It is also the cause of the order-2 solve with R′ off by about -2e7 that I mentioned on #483.Change
propagating_modesandselect_line_modetakemax_index: float | None = None. When set, modes with|n_eff| > max_indexare dropped. The default keeps the current selection unchanged, so existing calls behave as before.gsim.femwell.conductor_index_bound(epsilon, *, fraction=0.1)derives the bound from the permittivity passed tosolve_modes, either the per-region dict or per-element values. It returnsfractiontimes the smallest|sqrt(eps)|over regions where conduction dominates (|Im eps| > |Re eps|), counting only those whose bound is at leastmargin(default 10) times the largest index of the non-conducting regions. Weaker conductors, such as moderately doped silicon, are skipped with aUserWarning; when none qualifies it returnsNone, so the bound is never below the light line. It does not need the femwell runtime.solve_modesandselect_line_modedocstrings point to it.Usage:
Why an index bound
The issue proposed either a bound on
|n_eff|or a field-energy fraction in the dielectric.gsim.common.modesreads modes throughn_effalone, so it works for Palace rows, femwell modes and plain numbers. A field fraction would need fields and a region map that only femwell has. The two kinds of mode are far apart. On a quasi-TEM line over a good conductor,n_line**2 / eps_metalis abouteps_d R' eps0 / (L' sigma), which does not depend on frequency and is of order 1e-7 here: the line mode has|n_eff|around 7, and the spurious modes in #498 start at 0.89 times|sqrt(eps_metal)|. The default fraction of 0.1 leaves a wide margin on both sides. For lines over doped silicon, a caller can move it withfraction=.The bound is opt-in because
select_line_modehas no material information, and changing its default would need a released warning first.Tests
tests/common/test_mode_selection.py::TestIndexBound: the spurious index from select_line_mode can pick a spurious metal mode when more modes are requested #498 (23174-12017j) wins without a bound and is dropped with one; the bound applies to the magnitude and is inclusive;NoLineModeErroris raised when nothing survives.tests/femwell/test_adapter.py::TestConductorIndexBound: dict and per-element forms, the least conductive strong conductor sets the bound, a weak conductor (11.7-12j) alone givesNonewith a warning and does not lower an aluminium bound, the margin is taken against the largest dielectric index, a lossy dielectric is not a conductor,fractionandmarginvalidation.tests/femwell/test_series_resistance.py::TestManyModes: the mesh from select_line_mode can pick a spurious metal mode when more modes are requested #498 (25 nm at the strip, oxide capped at 0.5 µm), order 2, 12 modes. With the bound, R′ is within 3e-3 of the analytic DC resistance1/(sigma W t).